DEP-161: Mask sensitive information in API logs - #2733
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2733 +/- ##
==========================================
+ Coverage 74.89% 74.91% +0.01%
==========================================
Files 543 544 +1
Lines 20377 20451 +74
Branches 1578 1578
==========================================
+ Hits 15262 15321 +59
- Misses 5108 5123 +15
Partials 7 7
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
jareth-whitney
left a comment
jareth-whitney
left a comment
There was a problem hiding this comment.
Great job obfuscating sensitive data and updating logging behaviour! As usual, nothing blocking, just some things to check out before merging:
- Idea: wrap the print function and deprecate it, so IDE's can provide a nice little tooltip-style message to the coder
- Logging removed without replacement (likely intentional)
- Singular vs plural engagement/engagements
- Possible opportunities for map function to keep things concise
- Copyright 2021 vs 2026 (not sure which is right)
| def visit_call(self, node: nodes.Call) -> None: | ||
| """Check if the node is a print function call.""" | ||
| if isinstance(node.func, nodes.Name) and node.func.name == "print": | ||
| # Allow print in config.py for startup messages before logging is initialized |
| Print statements bypass logging masking and pose a security risk by potentially | ||
| exposing sensitive data (passwords, tokens, API keys, etc.) in logs. | ||
|
|
||
| Use current_app.logger or logging.getLogger(__name__) instead. |
There was a problem hiding this comment.
Here is a crazy idea, maybe we could wrap the print function in a user defined function that is deprecated, so the coder sees a pretty little on-hover suggestion to use current_app.logger when they try to use print.
import builtins
builtins.print = deprecated_print
| """Update scheduled engagements to published.""" | ||
| datetime_due = datetime.utcnow() | ||
| print('Publish due date (UTC) ------------------------', datetime_due) | ||
| logging.getLogger(__name__).debug('Publish due date (UTC): %s', datetime_due) |
There was a problem hiding this comment.
Specifiers bring me back to php <3
| template = {'template': ('template', file_handle, 'multipart/form-data')} | ||
|
|
||
| current_app.logger.info('Uploading template %s', template_file_path) | ||
| print('Uploading template %s', template_file_path) |
There was a problem hiding this comment.
Sometimes logging is just too verbose. I'm guessing it's intentional to have no replacement logging here.
There was a problem hiding this comment.
Yes; plus it exposed file path data which I couldn't think of a good way to parse separately from URL paths.
| True, | ||
| ) | ||
| print('Engagements published added to email queue: ', engagement.id) | ||
| current_app.logger.info('Engagement published added to email queue: %s', engagement.id) |
There was a problem hiding this comment.
Engagement/engagements, singular vs plural
| """ | ||
|
|
||
| # Mask string to replace sensitive data | ||
| MASK = '***REDACTED***' |
There was a problem hiding this comment.
Now we're working for the government!
| if any(sensitive in key.lower() for sensitive in sensitive_keys): | ||
| masked_data[key] = '***REDACTED***' | ||
| elif isinstance(value, dict): | ||
| masked_data[key] = mask_dict(value, sensitive_keys) |
| if not callable(value): | ||
| config_dict[attr] = value | ||
|
|
||
| masked_dict = mask_dict(config_dict) |
There was a problem hiding this comment.
Wondering if this could be achieved with a map.
| @@ -0,0 +1,82 @@ | |||
| # Copyright © 2021 Province of British Columbia | |||
There was a problem hiding this comment.
I'm not sure if the date should stay at 2021 or be 2026, just flagging it in case
| msg=normal_message, args=(), exc_info=None | ||
| ) | ||
| filter_obj.filter(log_record) | ||
| assert log_record.msg == normal_message |
There was a problem hiding this comment.
Thanks for adding tests as well.
|
4 participants
Issue #: 🎟️ DEP-161
Description of changes:
met_api.utils.logging_maskerfor implementation detailsUser Guide update ticket (if applicable):
Common component changes: